-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Milestones module development #343
base: master
Are you sure you want to change the base?
Conversation
Added layouts Added menu items Added classes, activities, fragments
Milestones list view
Remove redundant code
CRUD milestones
Create and edit milestone
View include: - title - due to date - description - progress - back to milestones list button
…Ber/ForkHub into iss8_milestine_view_page
Fixed the test
Added milestone support in old issue class
Fragment replace with updated milestone
Remove unnecessary string (login_or_email)
#39 translations
Merge commits from Jonan repository
Split layouts for different places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested everything yet, but I've added a few comments so you can start working on it if you want.
@@ -34,7 +34,7 @@ | |||
<string name="error_gists_load">Ошибка при загрузке Gists</string> | |||
<string name="error_issue_load">Ошибка при загрузке задачи</string> | |||
<string name="error_collaborators_load">Ошибка при загрузке соучастников</string> | |||
<string name="error_milestones_load">Ошибка при загрузке целей</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't update all the RU strings in this PR. You can send a new PR with this changes, but limit this one to just the new strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in phansier@e3a85c2
If OK, will be merged to PR
Moshi converter = new Moshi.Builder() | ||
.add(new DateAdapter()) | ||
.add(Milestone.class, adapter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a custom adapter for milestones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To allow setting null due_date for milestone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you achieve the same result by modifying the current DateAdapter
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, default adapter for milestones just ignores null properties.
So there is no other way to send patch request with due_on = null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean changing the behavior of https://github.com/jonan/ForkHub/blob/master/app/src/main/java/com/github/mobile/api/DateAdapter.java which is already being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set due_on = null, then DateAdapter is not being called by Milestone adapter because it skips null properties.
Yes, we can achieve it only with DateAdapter, if use some date as null. For example, DateAdapter will replace 01/01/1990 to null. But I am not sure, that it is a better alternative.
assertFalse(filter1.equals(filter2)); | ||
filter2.setMilestone(milestone); | ||
filter2.setMilestone(extraMilestone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done removing the old org.eclipse.egit.github.core.Milestone
code, simply creating the milestone with the new model object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in phansier@9e5ae56
If OK, will be merged to PR
if (milestone != null){ | ||
issue.setMilestone(milestone.getOldModel()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace missing before {
😉
And please remove last line break for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in phansier@30048f0
If OK, will be merged to PR
Call<List<Issue>> getIssues( | ||
@Path("owner") String owner, | ||
@Path("repo") String repo, | ||
@Query("milestone") long milestone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More pagination problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer to fix it in future because it is not the vital problem for milestones
app/src/main/java/com/github/mobile/ui/issue/IssuesViewActivity.java
Outdated
Show resolved
Hide resolved
app/src/main/res/menu/repository.xml
Outdated
<item | ||
android:id="@+id/m_milestone" | ||
app:showAsAction="never" | ||
android:title="@string/milestone"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use the plural "Milestones" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in phansier@f493fe9
If OK, will be merged to PR
else | ||
setGone(2, true); | ||
|
||
setChecked(0, selected == position); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why delete this code?
You're breaking the issue filtering by milestone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed together with UI fixes phansier@d3658ad
If OK, will be merged to PR
app/src/main/java/com/github/mobile/ui/issue/EditIssuesFilterActivity.java
Outdated
Show resolved
Hide resolved
Ui changes
PR comments fix
… into pr_comments_fix
Revert Incorrect transition between issue and milestone activities
Created:
Made other changes related to Milestones
Changed some russian (ru) localisation strings.